Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split generated code into two types #282

Merged
merged 12 commits into from Aug 22, 2022

Conversation

KodrAus
Copy link
Member

@KodrAus KodrAus commented May 25, 2022

Closes #168
Closes #155
Closes #218
Closes #263
Closes #228

Following on from #264

Part of #262

This PR is a refactoring of the generated flags types to make the API ownership between us (bitflags) and them (callers of bitflags!) clearer. We generate a public type with an API that we can't make a lot of change to outside of the BitFlags trait, and a private type that can hold trait implementations that users may choose to bubble up publicly.

This makes it possible to pick different semantics for traits like Ord, and add optional support for external libraries like Arbitrary.

It comes with some breakage though:

  • If you previously had a #[derive(Serialize)] impl, you'll need a #[serde(transparent)] attribute to ensure your format doesn't change.
  • If you #[derive(Debug)], you'll now get your flags wrapped in the name of your type (newtype formatting).
  • The .bits field is now a .bits() method.

We'll need to give this a thorough test once it builds to make sure it's workable for all the places bitflags is currently used, but I think this is a promising future direction for the project. Kind of like the lazy_static crate, a lot of our old complexity goes away when we don't need to work around some old language limitations.

src/lib.rs Outdated Show resolved Hide resolved
@@ -382,77 +432,83 @@ macro_rules! bitflags {
}
}

const _: () = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for putting this in a const _: () = { ... }?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out const _: () = { .. } is a great container for code generated by macros because:

  1. It's supported in any context where other items can be generated. You can write them in modules, functions, and the middle of other expressions.
  2. It gives us a new scope to define items in. Any types we define in the body of this const don't conflict with any outside of it. Any types we define in the const are also unreachable from outside of it. That's why we've got a trait with an associated type that lets us peek inside. We can even wrap the contents of the const body in a mod to hide fields if we want.

@KodrAus KodrAus mentioned this pull request Aug 11, 2022
@KodrAus
Copy link
Member Author

KodrAus commented Aug 22, 2022

cc @konsumlamm @arturoc What do you all think of this new internal library organization? Are you happy with this direction?

@konsumlamm
Copy link
Contributor

I like it! No complaints from me.

@arturoc
Copy link

arturoc commented Aug 22, 2022

looks good to me

@KodrAus
Copy link
Member Author

KodrAus commented Aug 22, 2022

Alrighty! I think the only thing remaining before we could consider stabilizing is adding an associated type for the iterator to the BitFlags trait, so that you can name it.

I'll do that in a follow-up, then look at publishing a pre-release build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants